Skip to content

Preserve federated quantiles across empty client digests - #5364

Merged
YuanTingHsieh merged 2 commits into
NVIDIA:mainfrom
sylvesterkaczmarek:fix/preserve-quantiles-across-empty-clients
Oct 7, 2026
Merged

Preserve federated quantiles across empty client digests#5364
YuanTingHsieh merged 2 commits into
NVIDIA:mainfrom
sylvesterkaczmarek:fix/preserve-quantiles-across-empty-clients

Conversation

@sylvesterkaczmarek

@sylvesterkaczmarek sylvesterkaczmarek commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Fixes #5363.

Preserve the accumulated t-digest when a client supplies no digest, and replace an empty placeholder when a later client has data. This prevents client-order-dependent loss of quantiles and AttributeError from calling .merge() on an empty dictionary. All-empty features continue returning unavailable quantiles.

The regression exercises the public get_quantiles path using real fastdigest objects. It covers all six orders of two populated clients and one empty client, checks expected quantiles and unchanged serialized inputs, and retains an all-empty control.

Validation

python -m pytest -q tests/unit_test/app_common/statistics

92 passed on Linux ARM64, Python 3.12, with the project's pinned fastdigest==0.4.0. The focused regression on unchanged upstream gives 6 failures and 1 pass. Black, isort, flake8 and git diff --check pass for the changed files.

This was tested against the checkout's source in an isolated Linux container. A deployed federated job was not run.

@sylvesterkaczmarek
Preserve federated quantiles across empty client digests
67f9aae 
Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
@greptile-apps

greptile-apps Bot commented Oct 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium risk] Fixes quantile merging logic for empty client digests.

The quantile fix appears safe to merge.

Summary

The PR preserves accumulated federated quantiles when a client supplies an empty digest, replaces an empty placeholder when a later client supplies data, and adds tests for client ordering and all-empty inputs.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Client feature digest] --> B{Contains digest data?}
  B -- No --> C[Keep existing digest or create empty placeholder]
  B -- Yes --> D{Accumulated digest has data?}
  D -- No --> E[Store client digest]
  D -- Yes --> F[Merge client digest]
  C --> G[Compute quantiles]
  E --> G
  F --> G
Loading

Reviews (2) · Last reviewed commit: "Merge branch 'main' into fix/preserve-qu..." · Reviewed by Greptile

This was referenced Oct 7, 2026
Closed
Closed
YuanTingHsieh
YuanTingHsieh approved these changes Oct 7, 2026
View reviewed changes

@YuanTingHsieh YuanTingHsieh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, LGTM

YuanTingHsieh
YuanTingHsieh approved these changes Oct 7, 2026
View reviewed changes

@YuanTingHsieh YuanTingHsieh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix correctly preserves accumulated digests when a client supplies no digest and replaces empty placeholders when valid data arrives. The regression tests cover all six client orders and retain the all-empty control. LGTM.

@sylvesterkaczmarek

sylvesterkaczmarek commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @YuanTingHsieh for the review and approval. I see the updated test matrix running on the approved head and will leave the change unchanged for the CI results.

@YuanTingHsieh
YuanTingHsieh added this pull request to the merge queue Oct 7, 2026
@codecov-commenter

codecov-commenter commented Oct 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.35%. Comparing base (32f5999) to head (8a22a0b).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
nvflare/app_opt/statistics/quantile_stats.py 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #5364   +/-   ##
=======================================
  Coverage   69.35%   69.35%           
=======================================
  Files        1031     1031           
  Lines      108680   108680           
=======================================
+ Hits        75372    75373    +1     
+ Misses      33308    33307    -1
Flag Coverage Δ
unit-tests 69.35% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Merged via the queue into NVIDIA:main with commit c78e210 Oct 7, 2026
23 checks passed
@songyinggoh songyinggoh mentioned this pull request Oct 8, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@YuanTingHsieh YuanTingHsieh YuanTingHsieh approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Empty client digests overwrite accumulated federated quantiles

3 participants

@sylvesterkaczmarek @codecov-commenter @YuanTingHsieh